Skip to content

remove redundant "else" test in temperature example #451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 8, 2019

Conversation

Artoria2e5
Copy link
Contributor

Fix #450. There is also a spelling problem with "precede".

Fix arduino#450. There is also a spelling problem with "precede".
{
//Warning! User attention required
// Warning! Thorttle down, run the fans, and notify the user
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You introduced a typo here (should be spelled "Throttle"). I'm actually not convinced changing this comment provides any benefit, though I do very much agree with the change to a consistent comment style.

@robsoncouto
Copy link
Contributor

robsoncouto commented Oct 4, 2018

I would recommend not removing the redundant part of the expression. The code without it may be counter-intuitive for beginners. Besides, if you add more cases (temperature ranges) to the example, you probably would have to put it back.

I instead recommend just reorganizing the code as follows:

if (temperature < 60)
{
  // Safe! Continue usual tasks...
}
else if (temperature >= 60 && temperature < 70)
{
  // Warning! Throttle down, run the fans, and notify the user
}
else if (temperature >= 70)
{
   // Danger! Shut down the system
}

@Daniel-1276
Copy link

I am a beginner and raised the related issue. My point in reading the example was to understand what the difference between "if" and "else if" is. And I still think, the current example does not show the benefit of not having to use redundant checks, if the "above" statements were already checked. In the example of @robsoncouto you could also just use "if" statements IMHO.

What about using an example something like this:
if (temperature < 60)
{
status=good; //means temperature is below 60
}
else if (temperature <90)
{
status=GettingHot; //means temperature is between 60 and 89
}
else if(temperature <100)
{
status=DangerZone; //means temperature is between 90 and 99
}
else
{
status=Alarm; //means temperature is above 99
}

@robsoncouto
Copy link
Contributor

Hey @per1234 , could you please fix this one as you think it is adequate?
I think that the else example should demonstrate well the concept of mutual exclusivity and not focus on saving a few instructions. This is just my opinion, though.

@per1234
Copy link
Collaborator

per1234 commented Jun 2, 2019

My concern with the current code and your suggested code even more so is not about efficiency, but ease of code maintenance. Let's imagine the programmer decides they want to reduce the danger temperature threshold to 65. With your suggested code, they need to remember to change the value in two different places. If they forget to change one of the two values (a very easy thing to do when you have a lot of code in the clause):

if (temperature < 60)
{
  // Safe! Continue usual tasks...
}
else if (temperature >= 60 && temperature < 70)
{
  // Warning! Throttle down, run the fans, and notify the user
}
else if (temperature >= 65)
{
   // Danger! Shut down the system
}

then the intended change in behavior was not accomplished.

Of course if you used variables instead it wouldn't be an issue, but adding variables to the example code introduces extra complexity, which is contrary to your intent of making it easier for a beginner to understand.

My suggestion is to use best practices in the code, but add comments to make it clear how the code works. Something like this:

if (temperature >= 70)
{
  // Danger! Shut down the system
}
else if (temperature >= 60)  // 60 <= temperature < 70
{
  // Warning! User attention required
}
else  // temperature < 60
{
  // Safe! Continue usual tasks...
}

@robsoncouto robsoncouto merged commit e26538a into arduino:master Jun 8, 2019
@robsoncouto
Copy link
Contributor

Updated and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve "else" example
4 participants